Phase-2 completion: post-process context types, m_riemann_solvers split, cleanup by sbryngelson · Pull Request #1556 · MFlowCode/MFC

sbryngelson · 2026-06-11T08:35:14Z

Stacked on #1551 → #1552 → #1553 → #1555 — its own content is the top 10 commits. Completes the refactoring roadmap's Phase 2 using the patterns the pilots (#1555) validated.

Summary

Context types (post_process). The remaining module-level state bundles follow the pilot pattern: fd_context (finite-difference workspace: gm_rho_sf, fd_coeff_x/y/z) and output_context (the 18-member output workspace: q_sf family, Silo extents/offsets, paths and handles). Strictly NFC, verified by emitted-Fortran equivalence after stripping the qualifier.
m_riemann_solvers.fpp split (4,706 lines → 6 modules). The four solvers extract to m_riemann_solver_{hll,hllc,hlld,lf}.fpp; the shared device state (14 GPU-declared arrays/bounds) and the per-sweep lifecycle + viscous helpers the solvers call land in m_riemann_state.fpp (the lowest layer — keeping them in the dispatching core would create a module cycle); the original module shrinks to a 116-line dispatcher that re-exports the public surface unchanged (zero external callers modified). Pure motion: statement-multiset equivalence under CPU/OpenACC/OpenMP emission; GPU-directive census identical (747 directives); every declare target verified to live in its declaring module (the Cray ftn-1448 rule).
Two latent bug fixes (found by review during the above):
- post_process's finite-difference coefficient arrays were never allocated when only qm_wrt is set, while the coefficient computation runs for it — unallocated access for Q-criterion-only cases. The allocation guards now mirror the call sites.
- (In the cleanup batch) the case validator accepted patch_ib counts above the namelist array bound — carried in Share the global-parameters core across executables; generate MPI broadcasts; build hygiene #1552's follow-ups; noted here because the registry-driven message change rides this stack.
Cleanup batch: the recon_type validator message now derives from the parameter registry (the last hardcoded vocabulary site); fypp #:for set-literals become list-literals (emission order was Python-hash-dependent across processes — builds are now order-reproducible); one stale import removed; the slimmed Riemann core drops two unused imports.

Verification

Full golden suite (4-way sharded) green twice: once on the context/cleanup layer, once on the complete tip with the Riemann split.
Per-commit batteries on the split: statement-multiset union equivalence (seed-pinned, three emission configs), GPU-directive multiset equality, declare-target scoping checks with a negative control, identifier sweeps, toolchain pytest.
Independent adversarial review of every task; the split's review reproduced the call-graph that forced the state-module layout and the full equivalence evidence.

Notes for reviewers

This is MFC's largest GPU surface; the Frontier/NVHPC matrix on this PR is the decisive device-path check (all local proofs are emission-level by design — the working model runs no local GPU builds).
Pre-existing, deliberately untouched (noted for future cleanup): Gs_rs/Res_gs allocate without a finalize-side deallocate (base-identical), and the single-precision q_sf_s triple is never deallocated.
m_riemann_state holds the per-sweep helpers as well as state — the call graph forces it; the module doc enumerates the contents.

Issues fixed

Fixes #1564 (Q-criterion-only post cases access unallocated FD coefficient arrays)

Tracked, not fixed here: #1568 and #1569 (deallocate gaps documented in the review notes above).

Copilot

Pull request overview

Completes Phase 2 of the refactoring roadmap by (1) moving remaining post-process module state into context derived types, (2) splitting the large simulation Riemann solver module into smaller units while preserving the public surface, and (3) finishing the parameter-registry driven generation of Fortran declarations, case-opt declarations, and per-target MPI broadcast fragments (plus associated cleanup and a couple of latent bug fixes).

Changes:

Add registry-driven generation for additional Fortran include fragments (generated_case_opt_decls.fpp, generated_bcast.fpp) and extend typed derived-type namelist declarations via TYPED_DECLS.
Introduce new shared Fortran module m_global_parameters_common and update build system/CMake to regenerate includes at build time.
Replace remaining literal enums with generated named constants across several simulation modules; add post-process fd_context/output_context and update post-process code to use them.

Reviewed changes

Copilot reviewed 53 out of 56 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
toolchain/mfc/params/generators/fortran_gen.py	Extend Fortran codegen to emit typed decls, case-opt decl blocks, and per-target MPI broadcast include fragments.
toolchain/mfc/params/generators/cmake_gen.py	Update generator script header comment to match build-time invocation.
toolchain/mfc/params/descriptions.py	Remove descriptions for deregistered derived-type members.
toolchain/mfc/params/definitions.py	Rename IB patch namelist bound constant; document/avoid dead registrations; add `TYPED_DECLS`.
toolchain/mfc/params_tests/test_fortran_gen.py	Add/adjust tests for typed decl emission, case-opt decl generation, and generated broadcast output.
toolchain/mfc/lint_docs.py	Derive allowed doc tokens from analytic intrinsics and registry constraints instead of a hardcoded skiplist.
toolchain/mfc/case_validator.py	Make `recon_type` validation message registry-driven (choices/names from `CONSTRAINTS`).
toolchain/mfc/build.py	Change build slug to a human-readable prefix plus existing 10-hex hash.
src/simulation/m_viscous.fpp	Replace reconstruction-type literals with generated named constants.
src/simulation/m_thinc.fpp	Replace interface-compression literal with named constant.
src/simulation/m_start_up.fpp	Use named recon-type constants; make Fypp loop literal order deterministic.
src/simulation/m_riemann_solver_hlld.fpp	New module for the HLLD solver extracted from the former monolith.
src/simulation/m_rhs.fpp	Use named constants for interface compression and reconstruction types.
src/simulation/m_qbmm.fpp	Use named constants for bubble-model choices.
src/simulation/m_muscl.fpp	Use named constants for MUSCL order/limiter choices.
src/simulation/m_mpi_proxy.fpp	Switch large portions of MPI broadcast logic to `generated_bcast.fpp` include; keep manual residue.
src/simulation/m_data_output.fpp	Use named constant for precision selection.
src/simulation/m_checker.fpp	Use named constants for recon type and MUSCL order checks.
src/simulation/m_cbc.fpp	Use named constants for recon type; make Fypp loop literal order deterministic.
src/simulation/m_bubbles.fpp	Use named constants for bubble-model choices.
src/simulation/m_bubbles_EL.fpp	Use named constant for precision selection in formatted output.
src/simulation/include/inline_riemann.fpp	Use named constants for average-state selection.
src/pre_process/m_start_up.fpp	Update IC state references to use the new `ic_context` bundle.
src/pre_process/m_mpi_proxy.fpp	Switch large portions of MPI broadcast logic to `generated_bcast.fpp` include; keep manual residue.
src/pre_process/m_initial_condition.fpp	Replace module-level IC state with `type(ic_context) :: ic`.
src/pre_process/m_data_output.fpp	Use named constant for precision selection.
src/pre_process/m_boundary_conditions.fpp	Remove stale/unused import.
src/post_process/m_start_up.fpp	Route FD coeff computation and output workspace through new contexts.
src/post_process/m_mpi_proxy.fpp	Switch large portions of MPI broadcast logic to `generated_bcast.fpp` include; keep manual residue; use named `format` constant.
src/post_process/m_derived_variables.fpp	Replace module-level FD work arrays with `type(fd_context) :: fd` and update allocation guards.
src/common/m_variables_conversion.fpp	Use named constant for average-state selection.
src/common/m_mpi_common.fpp	Use named constants for recon type and post-process format handling.
src/common/m_helper_basic.fpp	Use named constants for recon type when computing buffer size.
src/common/m_global_parameters_common.fpp	New shared global-parameters core and shared defaults/equation-index setup.
src/common/m_derived_types.fpp	Add `ic_context`, `fd_context`, and `output_context` derived types.
src/common/m_constants.fpp	Remove legacy recon-type constants and retain/clarify compile-time bounds constants.
docs/module_categories.json	Register new split modules in documentation categories.
docs/documentation/contributing.md	Update contributor guidance for build-time codegen and shared global parameters.
cmake/ParamsCodegen.cmake	New build-time codegen custom command/targets for generated `.fpp` includes.
cmake/MFCTargets.cmake	New target setup function and related configuration logic.
cmake/GPU.cmake	New consolidated GPU/compiler flags configuration.
cmake/Fypp.cmake	New consolidated Fypp preprocessing/HANDLE_SOURCES logic with generated-include dependencies.
.claude/rules/common-pitfalls.md	Update internal contributor/agent notes to reflect new shared global-params and codegen pipeline.

+auto-generated at build time (ninja-tracked custom command) from the `TYPED_DECLS` and `FORTRAN_ARRAY_DIMS`
+tables in `toolchain/mfc/params/definitions.py`. For a plain scalar registered with
+`_r()` / `_nv()` above, no manual Fortran edit is needed — reconfigure (`./mfc.sh build`)
+and the generated include in `m_global_parameters_common.fpp` (compiled per target) is updated
+automatically.


+After editing any generator or table, force regen by reconfiguring (`./mfc.sh build`) —
+cached builds compile stale includes.


codecov · 2026-06-11T11:17:28Z

Codecov Report

❌ Patch coverage is 54.67944% with 615 lines in your changes missing coverage. Please review.
✅ Project coverage is 60.94%. Comparing base (ac30c32) to head (adfea36).
⚠️ Report is 7 commits behind head on master.

Files with missing lines	Patch %	Lines
src/simulation/m_riemann_solver_lf.fpp	50.28%	141 Missing and 32 partials ⚠️
src/simulation/m_riemann_state.fpp	62.47%	112 Missing and 52 partials ⚠️
src/simulation/m_riemann_solver_hlld.fpp	6.08%	107 Missing and 1 partial ⚠️
src/post_process/m_data_output.fpp	35.50%	86 Missing and 3 partials ⚠️
src/simulation/m_riemann_solver_hll.fpp	77.07%	34 Missing and 24 partials ⚠️
src/post_process/m_derived_variables.fpp	30.43%	16 Missing ⚠️
src/post_process/m_start_up.fpp	83.72%	7 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1556      +/-   ##
==========================================
- Coverage   61.17%   60.94%   -0.24%     
==========================================
  Files          74       82       +8     
  Lines       20313    19922     -391     
  Branches     2961     2924      -37     
==========================================
- Hits        12427    12141     -286     
+ Misses       5870     5805      -65     
+ Partials     2016     1976      -40

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

sbryngelson · 2026-06-12T00:01:02Z

Both doc comments addressed — the passages predated the build-time generation command (added in #1552's build-hygiene work) and still described the configure-time era. Corrected text: a plain rebuild regenerates the includes automatically (the custom command is ninja-tracked against everything under toolchain/mfc/params/, generators included, via GLOB_RECURSE — so editing an existing generator file does retrigger); the one true reconfigure case is adding a new file there, since the dependency list is globbed at configure time. That caveat now appears where the misleading 'force regen by reconfiguring' advice was.

Both passages predated the build-time generation command and still said reconfigure; a plain rebuild regenerates automatically (the command is ninja-tracked against toolchain/mfc/params/), and only adding a new file there needs a reconfigure. Flagged by Copilot review on MFlowCode#1556.

…ops, stale import

The allocation guards in s_initialize_derived_variables_module omitted qm_wrt while the s_compute_finite_difference_coefficients call guards in m_start_up include it, so a case writing only the Q-criterion accessed unallocated arrays. Pre-existing; found during fd_context review. Guards now mirror the call sites exactly.

…n_state NFC, pure motion. The four solver bodies call s_initialize/finalize_riemann_solver, s_populate_riemann_states_variables_buffers, and s_compute_viscous_source_flux, so keeping those in the dispatching core would create a use-cycle once the solvers move out. They go to the new lowest layer together with the 14 GPU_DECLARE'd state items they consume (declares move with declarations; lowest-consumer rule, bc_buffers precedent). Module-level allocation, GPU updates, and deallocation stay in core's s_initialize/finalize_riemann_solvers_module via use-association. Statement-multiset union vs the parent differs only by module boilerplate; GPU-directive census md5 unchanged; declare-target scoping verified for both files.

NFC, pure motion (calibration commit for the solver extractions). The solver body accesses flux_rsx_vf, flux_src_rsx_vf, is1/2/3 by host association and calls the per-sweep lifecycle helpers, both now use-associated from m_riemann_state. Core re-exports s_hlld_riemann_solver through its existing public list. Statement-multiset union vs the pre-split file differs only by module boilerplate; GPU-directive census md5 unchanged; declare-target scoping verified for all files.

NFC, pure motion. State access stays host-associated via m_riemann_state; the inline_riemann include moves with the solver since its macros expand here (roe_avg pulls in get_species_enthalpies_rt, gas_constant, molecular_weights, hence the m_thermochem only-list). Core re-exports the entry point through its existing public list. Statement-multiset union vs the pre-split file differs only by module boilerplate; GPU-directive census md5 unchanged; declare-target scoping verified for all files.

NFC, pure motion. State access stays host-associated via m_riemann_state; inline_riemann macros expand here (low-Mach correction), and the solver references molecular_weights directly, hence the m_thermochem only-list. Core re-exports the entry point through its existing public list. Statement-multiset union vs the pre-split file differs only by module boilerplate; GPU-directive census md5 unchanged; declare-target scoping verified for all files.

NFC, pure motion; completes the m_riemann_solvers split. State access stays host-associated via m_riemann_state; hllc additionally uses m_bubbles (f_cpbw_KM), m_bubbles_EE (rs/vs/ps), and m_surface_tension (s_compute_capillary_source_flux). With the last solver body gone, core drops the use-lists whose final consumers moved (m_variables_conversion, m_bubbles, m_bubbles_EE, m_surface_tension, m_chemistry, m_thermochem, the m_constants only-list) and the inline_riemann include; core keeps the dispatcher, module init/finalize, and the public re-exports. Statement-multiset union vs the pre-split file differs only by module boilerplate; GPU-directive census md5 unchanged; declare-target scoping verified for all six files.

Review follow-up: m_mpi_proxy and m_helper_basic are unreferenced in the slimmed core under all configs.

Both passages predated the build-time generation command and still said reconfigure; a plain rebuild regenerates automatically (the command is ninja-tracked against toolchain/mfc/params/), and only adding a new file there needs a reconfigure. Flagged by Copilot review on MFlowCode#1556.

… in the Riemann solvers On AMD flang OpenMP offload, the cross-translation-unit read of the static declare-target Re_size inside the Riemann solver kernels is unreliable and codegen-dependent: some solver kernels read it as 0, which collapses the interface Reynolds number to dflt_real and silently disables viscosity (the 2D_viscous_shock_tube regression from #1556). Making Re_size allocatable only moved the bug -- it fixed HLL/HLLC but broke the LF (riemann_solver=5) and IBM viscous paths (the inversion). Instead keep Re_size static and capture it into a host-set firstprivate local (Re_size_loc) in s_hll/hllc/lf_riemann_solver, so the kernels never read it from declare-target device memory and are immune to the compiler bug. Verified on Frontier AMD flang: 2D_viscous_shock_tube + 1D/2D LF-viscous + 2D IBM-viscous all pass. Underlying amdflang bug filed: ROCm/llvm-project#2890, llvm/llvm-project#203711.

…iemann split)

…it, cleanup (MFlowCode#1556)

Copilot AI review requested due to automatic review settings June 11, 2026 08:35

Copilot started reviewing on behalf of sbryngelson June 11, 2026 08:35 View session

Copilot AI reviewed Jun 11, 2026

View reviewed changes

sbryngelson force-pushed the phase2-remainder branch from fdfeed0 to 1d6969e Compare June 11, 2026 17:09

sbryngelson force-pushed the phase2-remainder branch from 7bfe9c5 to 9717bee Compare June 12, 2026 02:55

sbryngelson added 10 commits June 11, 2026 23:19

src: bundle post_process finite-difference state into fd_context

e0036ca

src: bundle post_process output workspace into output_context

3ad17bd

cleanup: registry-driven recon_type validation, deterministic fypp lo…

b8969f9

…ops, stale import

src: drop unused imports from the riemann dispatcher core

563f776

Review follow-up: m_mpi_proxy and m_helper_basic are unreferenced in the slimmed core under all configs.

sbryngelson force-pushed the phase2-remainder branch from 9717bee to adfea36 Compare June 12, 2026 03:20

sbryngelson merged commit ac5774e into MFlowCode:master Jun 12, 2026
31 checks passed

sbryngelson mentioned this pull request Jun 16, 2026

Moving EL Bubbles with MPI Decomposition #1290

Draft

27 tasks

ChrisZYJ added a commit to ChrisZYJ/MFC_PR that referenced this pull request Jun 23, 2026

Merge upstream 4cebbb0 into hypo HLLD (re-graft onto MFlowCode#1556 r…

51b8e0e

…iemann split)

Cowsreal pushed a commit to Cowsreal/MFCMarkZhang that referenced this pull request Jun 24, 2026

Phase-2 completion: post-process context types, m_riemann_solvers spl…

e0aa71b

…it, cleanup (MFlowCode#1556)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Phase-2 completion: post-process context types, m_riemann_solvers split, cleanup#1556

Phase-2 completion: post-process context types, m_riemann_solvers split, cleanup#1556
sbryngelson merged 11 commits into
MFlowCode:masterfrom
sbryngelson:phase2-remainder

sbryngelson commented Jun 11, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

codecov Bot commented Jun 11, 2026 •

edited

Loading

Uh oh!

sbryngelson commented Jun 12, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

		After editing any generator or table, force regen by reconfiguring (`./mfc.sh build`) —
		cached builds compile stale includes.

Uh oh!

Conversation

sbryngelson commented Jun 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Verification

Notes for reviewers

Issues fixed

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

codecov Bot commented Jun 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sbryngelson commented Jun 12, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

sbryngelson commented Jun 11, 2026 •

edited

Loading

codecov Bot commented Jun 11, 2026 •

edited

Loading